Conversation
EffectAnalyzer previously considered atomic operations to be ordered only with operations that read or write Wasm memories. But they should be ordered with all operations that read or write the machine's memory, which includes things like globals, tables, and the GC heap. Fix the problem. One test is updated now that memory.grow (which is atomic) is ordered with global accesses.
| ;; CHECK-NEXT: ) | ||
| ;; CHECK-NEXT: (local.get $0) | ||
| ;; CHECK-NEXT: (local.get $1) | ||
| ;; CHECK-NEXT: ) |
There was a problem hiding this comment.
I guess this regressed for a good reason, though it's hard to tell - please add a targeted test for this.
There was a problem hiding this comment.
What's the best way to test effects.h? Is there an optimization that tries to reorder things in an especially predictable way?
There was a problem hiding this comment.
SimplifyLocals might be easiest. It does
(local.set $x (..value..))
(..effect..)
(local.get $x)
=>
;; local.set removed
(..effect..)
(..value..) ;; get replaced, value moved past effect, if that was valid
| ;; CHECK-NEXT: (local.get $0) | ||
| ;; CHECK-NEXT: (local.get $1) | ||
| ;; CHECK-NEXT: ) | ||
| (func $~lib/allocator/arena/__memory_allocate (; 6 ;) (type $3) (param $0 i32) (result i32) |
There was a problem hiding this comment.
Actually why did this regress? I don't see an atomic operation here...
There was a problem hiding this comment.
It's the memory.grow, as mentioned in the commit message.
There was a problem hiding this comment.
Wait, is memory.grow atomic even when atomics are disabled? That is the case in the test.
There was a problem hiding this comment.
(and that seems harmful for optimization of MVP code)
There was a problem hiding this comment.
Yes, it currently is. I agree we should fix that.
3 participants
EffectAnalyzer previously considered atomic operations to be ordered
only with operations that read or write Wasm memories. But they should
be ordered with all operations that read or write the machine's memory,
which includes things like globals, tables, and the GC heap. Fix the problem.
To avoid regressions on MVP programs that want to be able to freely reorder instructions like global.get and memory.grow (which is atomic, preventing such reordering), update the effects of all atomic instructions to be atomic only when operating on shared memory, including shared heap types. This is valid because the ordering of atomic operations on unshared locations can never synchronize with or be observed by operations on other threads.